fix: Add check table UUID to detect table replacement#2890
fix: Add check table UUID to detect table replacement#2890kevinjqliu merged 5 commits intoapache:mainfrom
Conversation
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
Thanks for adding this!
pyiceberg/table/__init__.py
Outdated
| # Only check UUID for existing tables, not new tables | ||
| if not isinstance(self, StagedTable): | ||
| self._check_uuid(response.metadata) | ||
|
|
There was a problem hiding this comment.
What is the scenario where the commit response has a different table uuid?
The commit request should include AssertTableUUID so I would expect the catalog to verify that
There was a problem hiding this comment.
looks like the java implementation has this check, https://github.com/apache/iceberg/blob/f8ee29e6eb8b5f33ea0e91fa4406a76643cb4ef6/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java#L289-L294
guess it doesnt hurt!
more context in apache/iceberg#14363 and apache/iceberg#14337
There was a problem hiding this comment.
Thanks for the review @Fokko and @kevinjqliu!
I was following the behavior of both implementations while REST is explicit about the check the BaseMetastore operations says eventually check. That being said, I can't think of a scenario where the commit check would catch something that Assert Table check wouldn't. So the commit check is purely defensive.
| catalog._check_endpoint(Capability.V1_DELETE_VIEW) | ||
|
|
||
|
|
||
| def test_table_uuid_check_on_commit(rest_mock: Mocker, example_table_metadata_v2: dict[str, Any]) -> None: |
There was a problem hiding this comment.
I'm not a big fan of mocking this out, since I think this should already work as @kevinjqliu pointed out. When performing the update, AssertTableUUID should ensure that no other process has dropped and recreated the table. The requirement will be asserted by the REST catalog on the server, or with {Hive,Sql,etc}Catalog it should be part of the code when we maintain a lock on the table.
There was a problem hiding this comment.
yea, as the other comment pointed out, this is purely defensive in case the catalog isnt doing the right thing 🤷
imo, it doesn't hurt to check again after the table response
I dont like the mocks either, but i dont see another way to test this logic haha
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! minor nit on the comment
cc @Fokko could u take a look at this when u get a chance?
Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
3 participants
Rationale for this change
This PR adds table UUID validation on refresh and commit to detect when a table has been replaced. For example, if a table is dropped and recreated with the same name, this prevents accidentally operating on a different table than expected.
Modeled after the Java implementation.
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L202-L209
Python was missing this check.
Are these changes tested?
Added some tests at the table and catalog level
Are there any user-facing changes?
no